refactor: modernize session management and token validation#1390
refactor: modernize session management and token validation#1390
Conversation
solracsf
force-pushed
the
refactorUserBackend
branch
2 times, most recently
from
44990c5 to
e1b4226
Compare
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Thanks a lot for refactoring this. Could you add a doc block comment for all the new methods with a global description? Especially if they change the overall behaviour of the backend.
|
@julien-nc will adress all your comments at once ASAP. |
|
@solracsf There is no rush 😁 |
solracsf
force-pushed
the
refactorUserBackend
branch
from
e1b4226 to
c392b41
Compare
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
solracsf
force-pushed
the
refactorUserBackend
branch
from
c392b41 to
7c966c9
Compare
|
|
||
| try { | ||
| // copy skeleton | ||
| $userFolder = Server::get(IRootFolder::class)->getUserFolder($userId); | ||
| \OC_Util::copySkeleton($userId, $userFolder); | ||
| } catch (NotPermittedException $ex) { | ||
| // read only uses | ||
| } catch (NotFoundException|NotPermittedException $e) { | ||
| $this->logger->warning('Could not set up user folder on first login', ['exception' => $e]); | ||
| } |
There was a problem hiding this comment.
This means the login does not crash anymore if something went wrong in copySkeleton. I'm not sure we want that. Wdyt?
There was a problem hiding this comment.
The old code had getUserFolder() outside the try block, catching only NotPermittedException from copySkeleton. So a NotFoundException thrown by getUserFolder() itself would have propagated up uncaught, potentially crashing first login.
The refactor moved getUserFolder() inside the try block and added NotFoundException to the catch. This means two failure modes are now silently handled that previously weren't:
getUserFolder()throwingNotFoundException(storage not found)copySkeletonthrowingNotFoundException
Whether this is the right call, it depends: if you want the login to proceed even if home folder setup fails, then this is correct and a net improvement. If you want setup failures to be hard errors that block login, you'd need to let them propagate. Given that NotPermittedException was already silently swallowed for "read only uses", continuing on NotFoundException seems consistent.
Your call here.
There was a problem hiding this comment.
If there are storage-related errors, it's bad anyway.
2 participants
This PR is a comprehensive audit and refactor, resolving bugs and performance problems identified through iterative code review. Refactor user session management and token validation logic for improved clarity and functionality. Introduce constants for session data and enhance error handling.